-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DOCS-1916: Add overwrite fragment mods #2622
Conversation
Overall readability score: 55.58 (🟢 +0)
View detailed metrics🟢 - Shows an increase in readability
Averages:
View metric targets
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! LGTM. @piokasar Would be great for you to take a look too to see if this looks good and if there are any other cases you think are important to show.
LGTM! Thank you for doing this!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so clean. Really great to read, thank you!!
@@ -78,7 +78,262 @@ For an example of adding a fragment to a machine, see the [Add a Rover Fragment | |||
## Modify the config of a machine that uses a fragment | |||
|
|||
When you modify a fragment, those changes are pushed to all machines that use that fragment. | |||
If you need to modify the config of just one machine that uses a fragment you can do the following: | |||
If you need to modify the config of just one machine that uses a fragment you have two options: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need to modify the config of just one machine that uses a fragment you have two options: | |
If you need to modify the config of a singular machine that uses a fragment you have two options: |
Suggestion for clarity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think singular sounds a little bit more formal than necessary and I think just one makes sense--I'd like to keep as-is if that's okay!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes complete sense!
5. Add any mods you'd like to apply. | ||
Click to view each example: | ||
|
||
{{%expand "Change the name and attributes of a component" %}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are so beautiful I teared up a bit.
docs/fleet/configure-a-fleet.md
Outdated
- Takes whatever the pin number for pin `b` was and assigns that value to the DIR pin. | ||
Deletes the `pins.b` field. | ||
|
||
_If you want to change the `name` of a component (for example, `motor1`), use `$set`, as shown in the change the name of a component example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_If you want to change the `name` of a component (for example, `motor1`), use `$set`, as shown in the change the name of a component example. | |
_To change the `name` of a component (for example, `motor1`), use `$set`, as shown in the change the name of a component example. |
Very optional suggestion for conciseness since there is more text in this section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll swap the order of these two sentences--see below
docs/fleet/configure-a-fleet.md
Outdated
Deletes the `pins.b` field. | ||
|
||
_If you want to change the `name` of a component (for example, `motor1`), use `$set`, as shown in the change the name of a component example. | ||
`$rename` is for changing the key, not the value._ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`$rename` is for changing the key, not the value._ | |
`$rename` is for changing attributes' keys, not their values._ |
or
`$rename` is for changing the key, not the value._ | |
`$rename` is for changing an attributes key, not its value._ |
Do you know if in the context of Viam $rename
can still operate on multiple attributes simultaneously so that you can rename multiple keys?
I thought it could be nice to make it clear/reiterate that you're talking about an attribute key and value just for clarity, what do you think though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes--if I understand your question correctly, the example here shows exactly that--two things being renamed simultaneously. I tested and it worked.
Yeah I can add that clarification. I'll also swap the order of these two italicized sentences, which I think will help with your comment above this too.
{ | ||
"fragment_id": "abcd7ef8-fa88-1234-b9a1-123z987e55aa", | ||
"mods": [ | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking back it this - I think when the command is the same, it shouldn't be repeated - just have it as:
"$set": {
"components.motor1.attributes.max_rpm": 1818,
"components.motor1.attributes.pins.a": 30,
"components.motor1.attributes.board": "local"
}
Unless you were able to get it to work this way . Going to test this right now though to double check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think they may have to be stacked if the command is the same within the same fragment_mods entry - there's a duplicate key error in the json and it discards the second.
Screen.Recording.2024-03-07.at.2.49.40.PM.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh interesting! That looks a lot cleaner. I didn't try combining them--I only tried it repeated, which did seem to work also for me I thought! I'll update them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh interesting 🤔 I wonder why mine didn't that's odd
"fragment_id": "abcd7ef8-fa88-1234-b9a1-123z987e55aa", | ||
"mods": [ | ||
{ | ||
"$set": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would have to happen here as well
"fragment_id": "abcd7ef8-fa88-1234-b9a1-123z987e55aa", | ||
"mods": [ | ||
{ | ||
"$rename": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stacking would have to be here too. Sorry I missed this earlier.
Also forgot to mention that the docs look really nice - thank you 😊
Co-authored-by: Naomi Pentrel <[email protected]>
You can view a rendered version of the docs from this PR at https://docs-test.viam.dev/2622 |
https://docs-test.viam.dev/2622/fleet/configure-a-fleet/#modify-the-config-of-a-machine-that-uses-a-fragment